streaming code used to hang when it was asked to "exit" - fixed now by puneith · Pull Request #635 · GoogleCloudPlatform/python-docs-samples

puneith · 2016-11-04T22:25:49Z

jerjou

Were you able to reproduce this bug? (I assume this is to address #608) I wasn't able to reproduce, which is why I hadn't responded to the bug yet..

jerjou · 2016-11-04T22:34:13Z

speech/grpc/transcribe_streaming.py


+    audio_stream.stop_stream()
+    audio_stream.close()
+


Was this move necessary? I'd prefer to have the function that opened the stream be responsible for closing it.

I agree with you. But, the thread hangs and doesn't come out of the while loop. So, I had to move here and close when I get out.

Ah - this is because buff.put(None) is never executed if the above loop is exited because stoprequest.is_set() is True. (this means the blocking buff.get() in _audio_data_generator blocks forever, since it's never given the None to exit from its loop).

If you change the above loop to:

try: ... except IOError: pass finally: buff.put(None)

then you should be able to put this back.

No, this happens because the stream is stopped and the thread hangs when this thread is still in the loop.

jerjou · 2016-11-04T22:34:45Z

speech/grpc/transcribe_streaming.py

-    audio_stream.stop_stream()
-    audio_stream.close()
    fill_buffer_thread.join()
-    audio_interface.terminate()


Any particular reason to remove this line? It seems like good hygiene..

That might have been a mistake. I will restore it.

jerjou · 2016-11-04T22:35:40Z

speech/grpc/transcribe_streaming.py

    """
-    while True:
-        # Use a blocking get() to ensure there's at least one chunk of data
+    while not stoprequest.isSet():


prefer .is_set, for python style conventions. Ditto below.

jerjou · 2016-11-04T22:40:51Z

speech/grpc/transcribe_streaming.py



-def _audio_data_generator(buff):
+def _audio_data_generator(buff, stoprequest):


_audio_data_generator shouldn't need this. Once _fill_buffer gets the signal, it'll add None to the buffer, which will cause this generator to exit.

But, fill_buffer doesn't add the None unless it receives IOError which it doesn't. Ok let me make one change to _fill_buffer.

Actually to think of it making both threads stop from the same signal is better.

Can you explain your reasoning?

The reasoning: Both reader and writer threads consider the stopsignal. If reader looks at the indirect signal from the writer, we don't know if it needed to stop because there is really a stop signal or an error in the writer's output.

Hm... sure, okay, I can see that. I think there's a case for just letting _fill_buffer propagate the way it is, but I won't block on it.

I actually had thought about both cases, but can now also see the case for other option. And now as I am thinking more I think the other option makes more sense :) Let me go back to it.

puneith · 2016-11-04T22:45:05Z

Yes I was able to reproduce this. It did hang on my mac.

jerjou · 2016-11-04T23:15:25Z

speech/grpc/transcribe_streaming.py

-    while True:
-        # Use a blocking get() to ensure there's at least one chunk of data
+    while not stoprequest.is_set():
+        # Use a blocking get() to ensure there's at least one chunk of data    


Please remove trailing whitespace

jerjou · 2016-11-04T23:24:39Z

speech/grpc/transcribe_streaming.py



-def _audio_data_generator(buff):
+def _audio_data_generator(buff, stoprequest):


Hm... sure, okay, I can see that. I think there's a case for just letting _fill_buffer propagate the way it is, but I won't block on it.

jerjou · 2016-11-04T23:40:00Z

speech/grpc/transcribe_streaming.py


+    audio_stream.stop_stream()
+    audio_stream.close()
+


Ah - this is because buff.put(None) is never executed if the above loop is exited because stoprequest.is_set() is True. (this means the blocking buff.get() in _audio_data_generator blocks forever, since it's never given the None to exit from its loop).

If you change the above loop to:

try: ... except IOError: pass finally: buff.put(None)

then you should be able to put this back.

jerjou · 2016-11-05T00:13:36Z

speech/grpc/transcribe_streaming.py

    """
    while True:
-        # Use a blocking get() to ensure there's at least one chunk of data
+        # Use a blocking get() to ensure there's at least one chunk of data    


Trailing whitespace

jerjou · 2016-11-05T00:17:13Z

speech/grpc/transcribe_streaming.py


    yield _audio_data_generator(buff)

-    audio_stream.stop_stream()


Any particular reason not to stop the stream before closing? While I'm not certain it's necessary, all the examples stop the stream before closing it.

Once the stream is stopped, one may not call write or read. And thread _fill_buffer gets stuck.

jerjou · 2016-11-05T00:22:59Z

speech/grpc/transcribe_streaming.py

    except IOError:
-        # This happens when the stream is closed. Signal that we're done.
-        buff.put(None)
+        pass


Without this, you'll have a race condition. ie if _audio_data_generator's buff.get() is waiting for data in the buffer when stoprequest is triggered, it could potentially wait forever and never yield a value.

jerjou

Nice - thanks for catching that race condition 😁
Looks good - just some polishing of the comments left.

jerjou · 2016-11-07T17:58:03Z

speech/grpc/transcribe_streaming.py

-        # This happens when the stream is closed. Signal that we're done.
+        pass
+    finally:
        buff.put(None)


A comment here describing what putting None means would be helpful.

jerjou · 2016-11-07T17:59:08Z

speech/grpc/transcribe_streaming.py

        # First, a thread that collects audio data as it comes in
-        with record_audio(RATE, CHUNK) as buffered_audio_data:
+
+        # stop request


Can you make this comment describe what stoprequest is for? Simply repeating the name of the variable isn't helpful.

jerjou · 2016-11-07T18:00:12Z

speech/grpc/transcribe_streaming.py

@@ -208,7 +212,11 @@ def main():
            make_channel('speech.googleapis.com', 443)) as service:
        # For streaming audio from the microphone, there are three threads.
        # First, a thread that collects audio data as it comes in


This comment should be right above the with block, since that's what it's describing.

jerjou · 2016-11-07T18:02:26Z

speech/grpc/transcribe_streaming.py

            except queue.Empty:
                break
+
+        # If the data contains None then set stop = True.


Comments should describe the meaning and purpose of the code, not just narrate what the code is doing. Maybe something like:

# If `_fill_buffer` adds `None` to the buffer, the audio stream is closed. # Yield the final bit of the buffer and exit the loop.

jerjou · 2016-11-07T18:03:09Z

speech/grpc/transcribe_streaming.py

+        # If the data contains None then set stop = True.
+        if None in data:
+            stop = True
+            data.remove(None)


Ah - nice catch.

jerjou · 2016-11-07T18:30:55Z

speech/grpc/transcribe_streaming.py


 def main():
+    # For streaming audio from the microphone, there are three threads.
+    # First, a thread that collects audio data as it comes in


Sorry - I meant that this comment should be on the below with statement, where it was before. ie since the comment is describing the thread that collects audio data, it should be just before the record_audio call.

jerjou · 2016-11-07T18:31:14Z

speech/grpc/transcribe_streaming.py

-        with record_audio(RATE, CHUNK) as buffered_audio_data:
+
+        # stoprequest is event object which is set in `listen_print_loop`.
+        # To indicate that the trancsription should be stopped.


trancsription -> transcription

jerjou · 2016-11-07T18:34:51Z

speech/grpc/transcribe_streaming.py

-        with record_audio(RATE, CHUNK) as buffered_audio_data:
+
+        # stoprequest is event object which is set in `listen_print_loop`.
+        # To indicate that the trancsription should be stopped.


Line 219 and 220 are the same sentence (otherwise, line 220 is a sentence fragment). Please change to:

# stoprequest is an event object which is set in `listen_print_loop` to indicate that the transcription should be stopped.

jerjou · 2016-11-07T19:32:06Z

speech/grpc/transcribe_streaming.py

+
+        # stoprequest is event object which is set in `listen_print_loop`.
+        # To indicate that the trancsription should be stopped.
+        # `_fill_buffer` checks and stops collecting data from audio_stream.


I had a bunch of trouble parsing this sentence. Perhaps:

# The `_fill_buffer` thread checks this object, and closes the `audio_stream` once it's set.

jerjou

Thanks!

puneith · 2016-11-07T19:54:20Z

@dpebot merge when green.

dpebot · 2016-11-07T19:54:21Z

Okay! I'll merge when all statuses are green.

puneith · 2016-11-07T19:55:36Z

Does dpebot know he can also merge when someone approves the PR + green status?

jerjou · 2016-11-07T19:59:04Z

I'm not sure what you're asking. Are you suggesting that we should merge PRs before the automated tests pass?

puneith · 2016-11-07T20:00:19Z

I am asking can dpebot take over when a) someone has approved the PR b) green status. Both a and b are true.

jerjou · 2016-11-07T20:04:21Z

I don't understand what you're saying. Are you stating that - currently - someone has approved this PR AND the PR's status is green? Or are you asking whether dpebot can submit the PR if a OR b are true? Or are you asking whether dpebot can submit the PR when a AND b are true?

jerjou · 2016-11-07T20:07:51Z

If it helps - IIUC, dpebot just waits for travis to turn green before submitting. Currently travis is still waiting for test results, so dpebot is not merging the PR yet.

puneith · 2016-11-07T20:10:36Z

I thought my question was simple :)

In other words I am asking. Do we have to ask dpebot explicitly "merge when everything is green"? Can dpebot not figure that out automatically and do the needful?

jerjou · 2016-11-07T20:33:03Z

I think your question is simple, but you're not expressing the full question, or you're not giving enough context for the question, so it's hard to figure out what you're asking. Perhaps it'd be helpful to structure your questions into three steps:

What the current situation is
What you expect the situation to be
The question

For example:

We ask dpebot to merge when green
dpebot should be able to do this without us asking
Do we need to ask dpebot, when we want to merge?

It was ambiguous in your original question what you thought the current situation was, and what you expected the situation to be. For instance - what did you mean by 'also'? That implies that there's another situation wherein dpebot will merge, besides when it's approved + green; but you don't describe this other situation you allude to, so there's no way for me to know what you're contrasting.

Does that make sense?

To answer your question - dpebot listens for travis to complete, and only merges if it has status:success AND the automerge label on it. Telling dpebot to 'merge when green' simply adds the automerge label. We don't want dpebot to just merge all green PRs because eg the user might still be working on the PR (for example, addressing comments from the reviewer), even if the tests pass.

puneith · 2016-11-07T23:48:23Z

Wow! Travis is sloooow!

jerjou · 2016-11-08T01:06:32Z

Yeah - it's usually one slow-running project in our org that backs everything else up :-/ We might consider changing to circleci, like some of the other teams are doing..

added streaming fix when exit

7b4bfc7

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 4, 2016

theacodes assigned jerjou Nov 4, 2016

puneith changed the title ~~added streaming fix when exit~~ streaming code used to hang when "exit" - fixed now Nov 4, 2016

puneith changed the title ~~streaming code used to hang when "exit" - fixed now~~ streaming code used to hang when it was asked to "exit" - fixed now Nov 4, 2016

jerjou suggested changes Nov 4, 2016

View reviewed changes

added audio_interface.terminate()

6e3a073

jerjou suggested changes Nov 4, 2016

View reviewed changes

removed audio_stream stop

1559d95

jerjou suggested changes Nov 5, 2016

View reviewed changes

puneithk added 2 commits November 4, 2016 19:35

removed trailing whitespace

d3be2f8

added None and checked for None in _audio_data_generator

a5d26c9

jerjou suggested changes Nov 7, 2016

View reviewed changes

added elaborate comments in the code

c0f8e5a

jerjou suggested changes Nov 7, 2016

View reviewed changes

fixed comments in the code

9648066

jerjou approved these changes Nov 7, 2016

View reviewed changes

puneith closed this Nov 7, 2016

puneith reopened this Nov 7, 2016

dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Nov 7, 2016

puneithk added 2 commits November 7, 2016 14:40

fixed comments

a891544

added 'the' back

decf575

dpebot merged commit 08b4da6 into master Nov 8, 2016

dpebot deleted the streaming_fix branch November 8, 2016 02:56

jerjou mentioned this pull request Nov 9, 2016

Use pyaudio's built-in async api. #645

Merged



		def _audio_data_generator(buff):
		def _audio_data_generator(buff, stoprequest):

Conversation

puneith commented Nov 4, 2016

Uh oh!

jerjou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puneith commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerjou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

puneith commented Nov 7, 2016 •

edited

Loading